Skip to content

Conversation

klausi
Copy link
Collaborator

@klausi klausi commented Apr 30, 2023

Issue: https://www.drupal.org/project/coder/issues/3214374

The goal would be to delete our FullyQualifiedNamespace sniff and switch to using the one from Slevomat.

Was not able to complete this yet because of a bug in Slevomat slevomat/coding-standard#1564

@jonathan1055
Copy link
Contributor

jonathan1055 commented May 1, 2023

You've probably realised this, but the failing tests for standards is because the new file tests/Drupal/good/GoodNamespace.php is not excluded when checking Coder's own files:

    <!-- Test files that should not be checked. -->
    <exclude-pattern>tests/*\.(inc|css|js|api\.php|tpl\.php)$</exclude-pattern>
    <exclude-pattern>tests/*/(good|bad)\.php$</exclude-pattern>
    <exclude-pattern>tests/*/drupal(6|7|8)/*\.php$</exclude-pattern>

The patterns above seem to exclude nearly all files in the tests/good/ folder but not everything. Am I right in thinking that all files in the tests/good folder should follow Drupal standards, and hence they are excluded because Coder uses a different customised set for it's own files? If so, we could exclude everything from tests/good in one statement, rather than try to cater for most files over the three patterns.

Sorry if this is wrong, I've not looked in great detail at what is not excluded from the above three.

@klausi
Copy link
Collaborator Author

klausi commented May 2, 2023

Yep, this is not ready for review yet, was just uploading my work in progress here.

@jonathan1055
Copy link
Contributor

Sorry, yes of course. But I've done some learning in the meantime, I wanted to see if we could exclude everything in /good except the GoodUnitTest.php but I could not get a neagative pattern to work, so the easiest thing would be to add GoodNamespace into the tests/*/(good|GoodNamespace|bad)\.php$ pattern

@klausi
Copy link
Collaborator Author

klausi commented May 6, 2023

Created upstream bug report at slevomat/coding-standard#1570

@klausi
Copy link
Collaborator Author

klausi commented May 6, 2023

That bug was resolved, yay!

Now some test fails remain where Slevomat gets confused by multiple use statements, it somehow thinks one is a constant.

Not sure if we should simply drop this test case from Coder and not care about it anymore, or if we should try to improve the Slevomat sniff.

@klausi
Copy link
Collaborator Author

klausi commented Jun 9, 2023

Reported the constant confusion upstream: slevomat/coding-standard#1581

@klausi
Copy link
Collaborator Author

klausi commented Jan 3, 2025

Giving up on this approach, as grouped use statements are still broken in Slevomat.

I'm fixing our existing sniff instead in #252 and #253 .

@klausi klausi closed this Jan 3, 2025
@jonathan1055
Copy link
Contributor

Yes that's a good plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants